Skip to content

Conversation

@fgalan
Copy link
Member

@fgalan fgalan commented Mar 5, 2021

This PR is not for being merged, it is just to expose publicly the branch. PR will be closed (and the branch deleted) after CI e2e testing. At the end code modification and test were added.

@fgalan fgalan changed the title FIX temporal change in iotagent-node-lib dependency [TMP] Testing NGSIv1 removal Mar 5, 2021
@jason-fox
Copy link
Contributor

@fgalan Once #561 , #558 and #553 have been merged you may want to pull the changes into this branch to see if the NGSI-v1 deletion is working as expected.

@fgalan
Copy link
Member Author

fgalan commented Jul 14, 2021

@fgalan Once #561 , #558 and #553 have been merged you may want to pull the changes into this branch to see if the NGSI-v1 deletion is working as expected.

After merging all these PRs and upgrade this PR branch with master, tests pass to ok. Thus, as far as I understand the NGSIv1 removal work in IOTA-JSON code has ended.

(Let's have this PR opened while the overall NGSIv1 removal work is completed)

@fgalan
Copy link
Member Author

fgalan commented Jul 14, 2021

Not sure if the NGSI-v1 removal work has ended in this repo... we have recently discovered isCurrentNgsi() usages in IOTA code. We are investigating it.

@jason-fox
Copy link
Contributor

The isCurrentNgsi() method hasn't been removed, but it will always be true, so the NGSI-v1 specific stuff in the JSON IoT Agent will be uncovered lava code. It is up to you which way round you want to break the code base:

  • iot-agent-node-lib doesn't support NGSI-v1 but the IoT Agent does.
  • IoT Agent doesn't support NGSI-v1 but iot-agent-node-lib does.

Obviously such a situation is only temporary.

@jason-fox
Copy link
Contributor

I have created (yet another) branch which removes the isCurrent() method - you could refer to it in the package.json to see where the code is going to break if isCurrent() doesn't exist.

https://github.com/jason-fox/iotagent-node-lib/tree/feature/isCurrent

@fgalan
Copy link
Member Author

fgalan commented Jul 14, 2021

@fgalan
Copy link
Member Author

fgalan commented Jul 14, 2021

The isCurrentNgsi() method hasn't been removed, but it will always be true, so the NGSI-v1 specific stuff in the JSON IoT Agent will be uncovered lava code. It is up to you which way round you want to break the code base:

  • iot-agent-node-lib doesn't support NGSI-v1 but the IoT Agent does.
  • IoT Agent doesn't support NGSI-v1 but iot-agent-node-lib does.

Obviously such a situation is only temporary.

As third alternative, we can use this PR branch (test-ngsiv1-removal) to remove NGSI-v1 IOTA code, re-scoping it's purpose (from just testing to actual code modification). Thus, it would be merged at the end (rolling back the modifications in packages.json before, as last step).

if (iotAgentLib.configModule.isCurrentNgsi()) {
return constants.TIMESTAMP_TYPE_NGSI2;
}
return constants.TIMESTAMP_TYPE;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If TIMESTAMP_TYPE constant is not used in any place, we could remove it from the source file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c904dfb

lib/iotaUtils.js Outdated
callback(error);
}

function extractAttributes(results, callback) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After simplifying this function, it has ended basically in a pass-through to the callback function.

Maybe we can simplify even more, removeing extractAttributes() function?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fd29166

return device.active[i].type;
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGES_NEXT_RELEASE entry is needed. Something like this:

- Remove: NGSI-v1 specific behaviours

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fd29166

@fgalan fgalan changed the title [TMP] Testing NGSIv1 removal [WIP] NGSIv1 removal Jul 14, 2021
Copy link
Member Author

@fgalan fgalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to code changes, although not ready to be merged yet (first packages.json should be changed back).

I understand same or similar changes has to be mirrored in PR telefonicaid/iotagent-ul#467

@fgalan fgalan changed the title [WIP] NGSIv1 removal NGSIv1 removal Sep 15, 2021
@fgalan
Copy link
Member Author

fgalan commented Sep 15, 2021

LGTM to code changes, although not ready to be merged yet (first packages.json should be changed back).

Change done in 286588d

Copy link
Collaborator

@mapedraza mapedraza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mapedraza mapedraza merged commit ed907bf into master Sep 15, 2021
@mapedraza mapedraza deleted the test-ngsiv1-removal branch September 15, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants